NE-2523: Implement configurationManagement API#1385
Open
Miciah wants to merge 2 commits intoopenshift:masterfrom
Open
NE-2523: Implement configurationManagement API#1385Miciah wants to merge 2 commits intoopenshift:masterfrom
Miciah wants to merge 2 commits intoopenshift:masterfrom
Conversation
openshift-ci-robot
added
the
jira/valid-reference
Contributor
|
@Miciah: This pull request references NE-2523 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
📝 WalkthroughWalkthroughAdds a 🚥 Pre-merge checks | ✅ 8✅ Passed checks (8 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pkg/operator/controller/ingress/deployment.go (1)
587-591:⚠️ Potential issue | 🟠 MajorUnsupported override can still force-enable DCM outside Dynamic mode
Line 590 assigns
enableDCM = v, sodynamicConfigManager: "true"can re-enable DCM even whenspec.tuningOptions.configurationManagementis notDynamic. That bypasses the new gate.Proposed fix
dynamicConfigOverride := unsupportedConfigOverrides.DynamicConfigManager -if v, err := strconv.ParseBool(dynamicConfigOverride); err == nil { - // Config override can still be used to opt out from DCM. - enableDCM = v +if v, err := strconv.ParseBool(dynamicConfigOverride); err == nil && !v { + // Config override can still be used to opt out from DCM. + enableDCM = false }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/operator/controller/ingress/deployment.go` around lines 587 - 591, The code currently sets enableDCM = v from unsupportedConfigOverrides.DynamicConfigManager, which lets a "true" override enable DCM even when spec.tuningOptions.configurationManagement is not "Dynamic"; change the logic so the parsed override only allows opting out: parse unsupportedConfigOverrides.DynamicConfigManager (as done in dynamicConfigOverride) and if parse succeeds and v == false then set enableDCM = false, but do not set enableDCM = true when v == true; keep all other logic intact (look for dynamicConfigOverride, unsupportedConfigOverrides.DynamicConfigManager and enableDCM to update this conditional).manifests/00-custom-resource-definition-OKD.yaml (1)
2120-2398:⚠️ Potential issue | 🔴 CriticalRegenerate this CRD with
spec.tuningOptions.configurationManagement.
spec.tuningOptionsstill does not defineconfigurationManagementanywhere in the schema. On a structural CRD, that means OKD will prune the new field from user objects, sospec.tuningOptions.configurationManagement: Dynamicnever reaches the operator.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@manifests/00-custom-resource-definition-OKD.yaml` around lines 2120 - 2398, The CRD schema for tuningOptions is missing the specification for spec.tuningOptions.configurationManagement so OKD will prune that field; add a new property named configurationManagement under the existing tuningOptions.properties (next to clientTimeout, maxConnections, etc.) with the proper type/enum (e.g., type: string and enum: ["Dynamic","Manual"] or the exact allowed values your operator expects), include any necessary validation/defaults, then regenerate the CRD so spec.tuningOptions.configurationManagement is persisted and reaches the operator.
🧹 Nitpick comments (2)
go.mod (1)
160-160: Add a follow-up comment next to theopenshift/apifork replace to ensure removal once upstream support is availableThe temporary fork replace at line 160 lacks any tracking comment. Add a
// TODO:or similar note indicating that this replace should be removed once upstream API changes are merged, so it doesn't get accidentally left in production.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@go.mod` at line 160, Add a short tracking comment next to the replace statement for the openshift API fork in go.mod (the line containing "replace github.com/openshift/api => github.com/Miciah/api v0.0.0-20260312135711-6a8c07c9cf16") — e.g., a "// TODO: remove replace when upstream openshift/api includes required changes (track PR/issue #...)" — so the temporary fork is clearly marked for removal once upstream support is available.pkg/operator/controller/ingress/deployment_test.go (1)
1208-1228: Add thedynamicConfigManager:true+ non-Dynamiccases to this matrix.The new table exercises
dynamicConfigManager:false, but not the inverse whenconfigurationManagementis omitted orForkAndReload. Those are the combinations that actually pin the new gating behavior and would catch regressions where the legacy override still enables DCM withoutDynamic.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/operator/controller/ingress/deployment_test.go` around lines 1208 - 1228, Add three mirror test cases to the existing table that use unsupportedConfigOverrides `{"dynamicConfigManager":"true","maxDynamicServers":"7"}` for the combinations where configurationManagement is omitted and where it is ForkAndReload (i.e., duplicate the blocks named "featuregate-enabled-configurationManagement-omitted-dcm-override" and "featuregate-enabled-configurationManagement-ForkAndReload-dcm-override" but with dynamicConfigManager set to true), setting dcmEnabled and expectedEnv to the values that assert DCM is enabled (the opposite of notSet). Ensure you update the entries that reference unsupportedConfigOverrides, dcmEnabled, configurationManagement, and expectedEnv so the table exercises the inverse/true path of the dynamicConfigManager flag.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@manifests/00-custom-resource-definition-OKD.yaml`:
- Around line 2010-2017: The CRD text wrongly states "TLS 1.3 cipher suites ...
are not configurable" which contradicts ingress behavior that renders and uses
ROUTER_CIPHERSUITES from a custom profile; update the prose under the "ciphers:"
example to accurately reflect behavior: explain that TLS 1.2 suites are
configurable, TLS 1.3 suites are negotiated differently but values provided in
the custom profile are rendered into ROUTER_CIPHERSUITES and will be respected
by the ingress controller where supported, or clearly state if TLS 1.3 suites
are ignored at runtime; adjust the sentence mentioning "not configurable" and
add a note referencing ROUTER_CIPHERSUITES and the custom profile to remove the
contradiction.
---
Outside diff comments:
In `@manifests/00-custom-resource-definition-OKD.yaml`:
- Around line 2120-2398: The CRD schema for tuningOptions is missing the
specification for spec.tuningOptions.configurationManagement so OKD will prune
that field; add a new property named configurationManagement under the existing
tuningOptions.properties (next to clientTimeout, maxConnections, etc.) with the
proper type/enum (e.g., type: string and enum: ["Dynamic","Manual"] or the exact
allowed values your operator expects), include any necessary
validation/defaults, then regenerate the CRD so
spec.tuningOptions.configurationManagement is persisted and reaches the
operator.
In `@pkg/operator/controller/ingress/deployment.go`:
- Around line 587-591: The code currently sets enableDCM = v from
unsupportedConfigOverrides.DynamicConfigManager, which lets a "true" override
enable DCM even when spec.tuningOptions.configurationManagement is not
"Dynamic"; change the logic so the parsed override only allows opting out: parse
unsupportedConfigOverrides.DynamicConfigManager (as done in
dynamicConfigOverride) and if parse succeeds and v == false then set enableDCM =
false, but do not set enableDCM = true when v == true; keep all other logic
intact (look for dynamicConfigOverride,
unsupportedConfigOverrides.DynamicConfigManager and enableDCM to update this
conditional).
---
Nitpick comments:
In `@go.mod`:
- Line 160: Add a short tracking comment next to the replace statement for the
openshift API fork in go.mod (the line containing "replace
github.com/openshift/api => github.com/Miciah/api
v0.0.0-20260312135711-6a8c07c9cf16") — e.g., a "// TODO: remove replace when
upstream openshift/api includes required changes (track PR/issue #...)" — so the
temporary fork is clearly marked for removal once upstream support is available.
In `@pkg/operator/controller/ingress/deployment_test.go`:
- Around line 1208-1228: Add three mirror test cases to the existing table that
use unsupportedConfigOverrides
`{"dynamicConfigManager":"true","maxDynamicServers":"7"}` for the combinations
where configurationManagement is omitted and where it is ForkAndReload (i.e.,
duplicate the blocks named
"featuregate-enabled-configurationManagement-omitted-dcm-override" and
"featuregate-enabled-configurationManagement-ForkAndReload-dcm-override" but
with dynamicConfigManager set to true), setting dcmEnabled and expectedEnv to
the values that assert DCM is enabled (the opposite of notSet). Ensure you
update the entries that reference unsupportedConfigOverrides, dcmEnabled,
configurationManagement, and expectedEnv so the table exercises the inverse/true
path of the dynamicConfigManager flag.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro
Run ID: cb9d4f60-f735-4342-a0c7-7455a5de6900
⛔ Files ignored due to path filters (170)
go.sumis excluded by!**/*.sumvendor/github.com/gogo/protobuf/AUTHORSis excluded by!**/vendor/**,!vendor/**vendor/github.com/gogo/protobuf/CONTRIBUTORSis excluded by!**/vendor/**,!vendor/**vendor/github.com/gogo/protobuf/LICENSEis excluded by!**/vendor/**,!vendor/**vendor/github.com/gogo/protobuf/proto/Makefileis excluded by!**/vendor/**,!vendor/**vendor/github.com/gogo/protobuf/proto/clone.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gogo/protobuf/proto/custom_gogo.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gogo/protobuf/proto/decode.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gogo/protobuf/proto/deprecated.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gogo/protobuf/proto/discard.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gogo/protobuf/proto/duration.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gogo/protobuf/proto/duration_gogo.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gogo/protobuf/proto/encode.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gogo/protobuf/proto/encode_gogo.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gogo/protobuf/proto/equal.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gogo/protobuf/proto/extensions.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gogo/protobuf/proto/extensions_gogo.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gogo/protobuf/proto/lib.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gogo/protobuf/proto/lib_gogo.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gogo/protobuf/proto/message_set.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gogo/protobuf/proto/pointer_reflect.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gogo/protobuf/proto/pointer_reflect_gogo.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gogo/protobuf/proto/pointer_unsafe.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gogo/protobuf/proto/pointer_unsafe_gogo.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gogo/protobuf/proto/properties.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gogo/protobuf/proto/properties_gogo.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gogo/protobuf/proto/skip_gogo.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gogo/protobuf/proto/table_marshal.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gogo/protobuf/proto/table_marshal_gogo.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gogo/protobuf/proto/table_merge.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gogo/protobuf/proto/table_unmarshal.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gogo/protobuf/proto/table_unmarshal_gogo.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gogo/protobuf/proto/text.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gogo/protobuf/proto/text_gogo.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gogo/protobuf/proto/text_parser.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gogo/protobuf/proto/timestamp.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gogo/protobuf/proto/timestamp_gogo.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gogo/protobuf/proto/wrappers.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gogo/protobuf/proto/wrappers_gogo.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gogo/protobuf/sortkeys/sortkeys.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/.ci-operator.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/.coderabbit.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/.golangci.go-validated.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/.golangci.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/AGENTS.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/Dockerfile.ocpis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/Makefileis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/apiextensions/install.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/apiextensions/v1alpha1/Makefileis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/apiextensions/v1alpha1/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/apiextensions/v1alpha1/register.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/apiextensions/v1alpha1/types_compatibilityrequirement.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/apiextensions/v1alpha1/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/apiextensions/v1alpha1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/apiextensions/v1alpha1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/apps/v1/generated.pb.gois excluded by!**/*.pb.go,!**/vendor/**,!vendor/**vendor/github.com/openshift/api/apps/v1/generated.protomessage.pb.gois excluded by!**/*.pb.go,!**/vendor/**,!vendor/**vendor/github.com/openshift/api/apps/v1/types.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/apps/v1/zz_prerelease_lifecycle_generated.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/authorization/v1/generated.pb.gois excluded by!**/*.pb.go,!**/vendor/**,!vendor/**vendor/github.com/openshift/api/authorization/v1/generated.protomessage.pb.gois excluded by!**/*.pb.go,!**/vendor/**,!vendor/**vendor/github.com/openshift/api/build/v1/generated.pb.gois excluded by!**/*.pb.go,!**/vendor/**,!vendor/**vendor/github.com/openshift/api/build/v1/generated.protomessage.pb.gois excluded by!**/*.pb.go,!**/vendor/**,!vendor/**vendor/github.com/openshift/api/cloudnetwork/v1/generated.pb.gois excluded by!**/*.pb.go,!**/vendor/**,!vendor/**vendor/github.com/openshift/api/cloudnetwork/v1/generated.protomessage.pb.gois excluded by!**/*.pb.go,!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/register.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/types_apiserver.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/types_authentication.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/types_cluster_image_policy.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/types_cluster_version.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/types_image_policy.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/types_infrastructure.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/types_ingress.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/types_insights.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/types_network.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/types_tlssecurityprofile.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/config/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/config/v1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/config/v1alpha1/register.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1alpha1/types_backup.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1alpha1/types_cluster_image_policy.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1alpha1/types_cluster_monitoring.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1alpha1/types_crio_credential_provider_config.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1alpha1/types_image_policy.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1alpha1/types_insights.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1alpha1/types_pki.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1alpha1/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/config/v1alpha1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/config/v1alpha1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/config/v1alpha2/types_insights.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1alpha2/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/console/v1/types_console_sample.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/envtest-releases.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/etcd/README.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/etcd/install.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/etcd/v1alpha1/Makefileis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/etcd/v1alpha1/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/etcd/v1alpha1/register.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/etcd/v1alpha1/types_pacemakercluster.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/etcd/v1alpha1/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/etcd/v1alpha1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/etcd/v1alpha1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/features.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/features/features.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/features/legacyfeaturegates.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/features/util.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/image/v1/generated.pb.gois excluded by!**/*.pb.go,!**/vendor/**,!vendor/**vendor/github.com/openshift/api/image/v1/generated.protomessage.pb.gois excluded by!**/*.pb.go,!**/vendor/**,!vendor/**vendor/github.com/openshift/api/install.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machine/v1/types_controlplanemachineset.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machine/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/machine/v1beta1/types_awsprovider.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machine/v1beta1/types_machine.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machine/v1beta1/types_machineset.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machine/v1beta1/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/machine/v1beta1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/network/v1/generated.pb.gois excluded by!**/*.pb.go,!**/vendor/**,!vendor/**vendor/github.com/openshift/api/network/v1/generated.protomessage.pb.gois excluded by!**/*.pb.go,!**/vendor/**,!vendor/**vendor/github.com/openshift/api/networkoperator/v1/generated.pb.gois excluded by!**/*.pb.go,!**/vendor/**,!vendor/**vendor/github.com/openshift/api/networkoperator/v1/generated.protomessage.pb.gois excluded by!**/*.pb.go,!**/vendor/**,!vendor/**vendor/github.com/openshift/api/oauth/v1/generated.pb.gois excluded by!**/*.pb.go,!**/vendor/**,!vendor/**vendor/github.com/openshift/api/oauth/v1/generated.protomessage.pb.gois excluded by!**/*.pb.go,!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/types_console.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/types_ingress.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/types_machineconfiguration.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/types_network.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_console_01_consoles.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers-CustomNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers-Default.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers-DevPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers-OKD.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_70_dns_00_dnses.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_70_network_01_networks.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_80_machine-config_01_machineconfigurations-CustomNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_80_machine-config_01_machineconfigurations-Default.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_80_machine-config_01_machineconfigurations-DevPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_80_machine-config_01_machineconfigurations-OKD.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_80_machine-config_01_machineconfigurations-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/operator/v1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/operator/v1alpha1/register.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1alpha1/types_clusterapi.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/operatoringress/v1/types.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operatoringress/v1/zz_generated.crd-manifests/0000_50_dns_01_dnsrecords-CustomNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operatoringress/v1/zz_generated.crd-manifests/0000_50_dns_01_dnsrecords-Default.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operatoringress/v1/zz_generated.crd-manifests/0000_50_dns_01_dnsrecords-DevPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operatoringress/v1/zz_generated.crd-manifests/0000_50_dns_01_dnsrecords-OKD.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operatoringress/v1/zz_generated.crd-manifests/0000_50_dns_01_dnsrecords-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operatoringress/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/operatoringress/v1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/project/v1/generated.pb.gois excluded by!**/*.pb.go,!**/vendor/**,!vendor/**vendor/github.com/openshift/api/project/v1/generated.protomessage.pb.gois excluded by!**/*.pb.go,!**/vendor/**,!vendor/**vendor/github.com/openshift/api/quota/v1/generated.pb.gois excluded by!**/*.pb.go,!**/vendor/**,!vendor/**vendor/github.com/openshift/api/quota/v1/generated.protomessage.pb.gois excluded by!**/*.pb.go,!**/vendor/**,!vendor/**vendor/github.com/openshift/api/route/v1/generated.pb.gois excluded by!**/*.pb.go,!**/vendor/**,!vendor/**vendor/github.com/openshift/api/route/v1/generated.protomessage.pb.gois excluded by!**/*.pb.go,!**/vendor/**,!vendor/**vendor/github.com/openshift/api/samples/v1/generated.pb.gois excluded by!**/*.pb.go,!**/vendor/**,!vendor/**vendor/github.com/openshift/api/samples/v1/generated.protomessage.pb.gois excluded by!**/*.pb.go,!**/vendor/**,!vendor/**vendor/github.com/openshift/api/security/v1/generated.pb.gois excluded by!**/*.pb.go,!**/vendor/**,!vendor/**vendor/github.com/openshift/api/security/v1/generated.protomessage.pb.gois excluded by!**/*.pb.go,!**/vendor/**,!vendor/**vendor/github.com/openshift/api/template/v1/generated.pb.gois excluded by!**/*.pb.go,!**/vendor/**,!vendor/**vendor/github.com/openshift/api/template/v1/generated.protomessage.pb.gois excluded by!**/*.pb.go,!**/vendor/**,!vendor/**vendor/github.com/openshift/api/user/v1/generated.pb.gois excluded by!**/*.pb.go,!**/vendor/**,!vendor/**vendor/github.com/openshift/api/user/v1/generated.protomessage.pb.gois excluded by!**/*.pb.go,!**/vendor/**,!vendor/**vendor/modules.txtis excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (8)
go.modmanifests/00-custom-resource-definition-CustomNoUpgrade.yamlmanifests/00-custom-resource-definition-DevPreviewNoUpgrade.yamlmanifests/00-custom-resource-definition-OKD.yamlmanifests/00-custom-resource-definition-TechPreviewNoUpgrade.yamlmanifests/00-custom-resource-definition.yamlpkg/operator/controller/ingress/deployment.gopkg/operator/controller/ingress/deployment_test.go
Contributor
|
/assign @candita |
jcmoraisjr
reviewed
Mar 18, 2026
| {"ROUTER_HAPROXY_CONFIG_MANAGER", true, "true"}, | ||
| {"ROUTER_MAX_DYNAMIC_SERVERS", true, "100"}, | ||
| {"ROUTER_BLUEPRINT_ROUTE_POOL_SIZE", true, "0"}, | ||
| }, |
Member
There was a problem hiding this comment.
Same Rabbit comment but on another place: is it expected DCM being enabled even without feature gate allowing it?
Contributor
Author
There was a problem hiding this comment.
Yes, unsupportedConfigOverride overrides the featuregate and API field.
| {"ROUTER_BLUEPRINT_ROUTE_POOL_SIZE", true, "0"}, | ||
| }, | ||
| }, | ||
| } |
Member
There was a problem hiding this comment.
Missed a test (trying to) adding configurationManagement and dcmEnabled as false.
Contributor
Author
There was a problem hiding this comment.
I will add the test cases. However, the API field is featuregated, so it does not exist (and therefore cannot be set) if the featuregate is not enabled.
Contributor
Author
There was a problem hiding this comment.
| if v, err := strconv.ParseBool(dynamicConfigOverride); err == nil { | ||
| // Config override can still be used to opt out from DCM. | ||
| enableDCM = v | ||
| } |
Member
There was a problem hiding this comment.
Similar to Rabbit's comment, and related with another one I added in the tests. Although this is not part of the change, but shouldn't this override be under the feature gate, just like the API?
Contributor
There was a problem hiding this comment.
I believe we enabled this as an unsupported option before we had the supported feature gate option. So we will still honor the unsupported override values even if the feature gate is disabled.
Contributor
Author
There was a problem hiding this comment.
As Candace said, the unsupported config override predates the featuregate. Besides that, unsupported config overrides generally do and should take precedence over any other configuration; that is implied by the word "override".
openshift-ci
bot
added
the
needs-rebase
Bump to github.com/Miciah/api@6a8c07c9cf163d3b5e7964ffb5c7d70f6bdeff5e
to get the spec.tuningOptions.configurationManagement API field to allow
enabling or disabling the Dynamic Configuration Manager.
go mod edit -replace github.com/openshift/api=github.com/Miciah/api@6a8c07c9cf163d3b5e7964ffb5c7d70f6bdeff5e
go mod tidy
go mod vendor
make update
* go.mod: Update.
* go.sum:
* manifests/00-custom-resource-definition.yaml:
* vendor/*: Regenerate.
Miciah
force-pushed
the
NE-2523-implement-configurationManagement-API
branch
from
f625550 to
048f2c8
Compare
Contributor
Author
Miciah
force-pushed
the
NE-2523-implement-configurationManagement-API
branch
from
048f2c8 to
60a6a81
Compare
openshift-ci
bot
removed
the
needs-rebase
Contributor
Author
|
https://github.com/openshift/cluster-ingress-operator/compare/048f2c85c55bd79de13c9595535727188c8812b0..60a6a81103e460aad99324da313859deeb74d579 adds some test cases in |
Miciah
force-pushed
the
NE-2523-implement-configurationManagement-API
branch
from
60a6a81 to
43139b2
Compare
Contributor
Author
|
...and https://github.com/openshift/cluster-ingress-operator/compare/60a6a81103e460aad99324da313859deeb74d579..43139b2c71b282878d605ce45172adab585d009e fixes the names of those new test cases (whoops!). |
Contributor
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/operator/controller/ingress/deployment_test.go (1)
1209-1246: Add one featuregate-enabled + override=true precedence case.You already assert override precedence in several paths; adding a
dynamicConfigManager:"true"case when featuregate is enabled andconfigurationManagementis omitted (orForkAndReload) would close the remaining high-value gap.Suggested test-case addition
{ name: "featuregate-enabled-configurationManagement-ForkAndReload-dcm-override", unsupportedConfigOverrides: `{"dynamicConfigManager":"false","maxDynamicServers":"7"}`, dcmEnabled: true, configurationManagement: operatorv1.IngressControllerConfigurationManagementForkAndReload, expectedEnv: notSet, }, + { + name: "featuregate-enabled-configurationManagement-omitted-dcm-override-true", + unsupportedConfigOverrides: `{"dynamicConfigManager":"true","maxDynamicServers":"7"}`, + dcmEnabled: true, + /* configurationManagement omitted. */ + expectedEnv: []envData{ + {"ROUTER_HAPROXY_CONFIG_MANAGER", true, "true"}, + {"ROUTER_MAX_DYNAMIC_SERVERS", true, "7"}, + {"ROUTER_BLUEPRINT_ROUTE_POOL_SIZE", true, "0"}, + }, + },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/operator/controller/ingress/deployment_test.go` around lines 1209 - 1246, Add a test-case to the existing table that verifies override=true precedence when the feature gate is enabled: insert a case similar to the other dcmEnabled:true entries with unsupportedConfigOverrides set to `{"dynamicConfigManager":"true"}`, dcmEnabled: true, leave configurationManagement omitted (or add a separate case with configurationManagement: operatorv1.IngressControllerConfigurationManagementForkAndReload), and set expectedEnv to the value that asserts the dynamicConfigManager override is honored (i.e., the env var indicating DCM is enabled). Target the test table entries using the fields unsupportedConfigOverrides, dcmEnabled, configurationManagement, and expectedEnv so the new case follows the existing "featuregate-enabled-..." group.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@go.mod`:
- Line 235: Remove the ad-hoc replace directive that points
github.com/openshift/api to a personal fork/commit (the replace line referencing
github.com/Miciah/api v0.0.0-20260312135711-6a8c07c9cf16); either revert the
replace to use the official upstream module/version or, if the forked commit is
temporarily required, replace it with a documented justification and a link to
the upstream PR plus an explicit removal condition (and a TODO) so the
dependency is not shipped indefinitely.
---
Nitpick comments:
In `@pkg/operator/controller/ingress/deployment_test.go`:
- Around line 1209-1246: Add a test-case to the existing table that verifies
override=true precedence when the feature gate is enabled: insert a case similar
to the other dcmEnabled:true entries with unsupportedConfigOverrides set to
`{"dynamicConfigManager":"true"}`, dcmEnabled: true, leave
configurationManagement omitted (or add a separate case with
configurationManagement:
operatorv1.IngressControllerConfigurationManagementForkAndReload), and set
expectedEnv to the value that asserts the dynamicConfigManager override is
honored (i.e., the env var indicating DCM is enabled). Target the test table
entries using the fields unsupportedConfigOverrides, dcmEnabled,
configurationManagement, and expectedEnv so the new case follows the existing
"featuregate-enabled-..." group.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 23b4b050-64b3-43c9-ba22-90956429be68
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (8)
go.modmanifests/00-custom-resource-definition-CustomNoUpgrade.yamlmanifests/00-custom-resource-definition-DevPreviewNoUpgrade.yamlmanifests/00-custom-resource-definition-OKD.yamlmanifests/00-custom-resource-definition-TechPreviewNoUpgrade.yamlmanifests/00-custom-resource-definition.yamlpkg/operator/controller/ingress/deployment.gopkg/operator/controller/ingress/deployment_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- manifests/00-custom-resource-definition-OKD.yaml
- manifests/00-custom-resource-definition.yaml
| // Remove this when sail-operator switches to an OSSM release branch. | ||
| replace sigs.k8s.io/gateway-api => sigs.k8s.io/gateway-api v1.4.1 | ||
|
|
||
| replace github.com/openshift/api => github.com/Miciah/api v0.0.0-20260312135711-6a8c07c9cf16 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Confirm the replace directive currently points to a personal fork.
rg -n '^\s*replace\s+github\.com/openshift/api\s*=>\s*github\.com/Miciah/api\s+v[0-9].*' go.mod
# 2) Extract the commit suffix from the pseudo-version.
COMMIT="$(rg -n '^\s*replace\s+github\.com/openshift/api\s*=>\s*github\.com/Miciah/api\s+v0\.0\.0-[0-9]+-([0-9a-f]{12})\s*$' -or '$1' go.mod)"
echo "commit suffix in go.mod: ${COMMIT}"
# 3) Verify whether this commit is reachable in upstream openshift/api.
# Expected: ideally found upstream before release; empty result means still fork-only.
git ls-remote https://github.com/openshift/api "${COMMIT}*"Repository: openshift/cluster-ingress-operator
Length of output: 216
Avoid shipping a replace directive that depends on a personal fork for github.com/openshift/api.
The commit 6a8c07c9cf16 in this replace does not exist in the upstream github.com/openshift/api repository, creating a supply-chain risk and potential for silent drift. This directive must be reverted before release or explicitly tied to a documented upstream PR with a clear removal condition.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@go.mod` at line 235, Remove the ad-hoc replace directive that points
github.com/openshift/api to a personal fork/commit (the replace line referencing
github.com/Miciah/api v0.0.0-20260312135711-6a8c07c9cf16); either revert the
replace to use the official upstream module/version or, if the forked commit is
temporarily required, replace it with a documented justification and a link to
the upstream PR plus an explicit removal condition (and a TODO) so the
dependency is not shipped indefinitely.
Contributor
Author
|
Before the rebase, ingress-operator was logging |
Check the spec.tuningOptions.configurationManagement API field, and only enable the Dynamic Configuration Manager if the field is set to "Dynamic". For now, the default behavior (when the field is omitted) is not to enable DCM. Once we are more confident that the feature is safe to enable, we will change the default behavior. This commit implements NE-2523. https://issues.redhat.com/browse/NE-2523 * pkg/operator/controller/ingress/deployment.go (desiredRouterDeployment): Check spec.tuningOptions.configurationManagement to enable DCM. * pkg/operator/controller/ingress/deployment.go (desiredRouterDeployment): Check that spec.tuningOptions.configurationManagement is set to "Dynamic" before enabling the Dynamic Configuration Manager. * pkg/operator/controller/ingress/deployment_test.go (TestDesiredRouterDeploymentDynamicConfigManager): Add the value for spec.tuningOptions.configurationManagement to the test input, and add test cases various different settings for configurationManagement.
Miciah
force-pushed
the
NE-2523-implement-configurationManagement-API
branch
from
43139b2 to
d12af90
Compare
Contributor
Author
|
https://github.com/openshift/cluster-ingress-operator/compare/43139b2c71b282878d605ce45172adab585d009e..d12af9099279bcdf071d559a8176335eb70bbfc2 fixes a formatting issue that cause the verify job to fail.. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/operator/controller/ingress/deployment_test.go (1)
1165-1207: Add one happy-pathmaxDynamicServerscase forConfigurationManagement=Dynamic.The matrix covers default, clamped, and invalid values, but not a valid positive override. A regression that ignores
maxDynamicServerson the new API-drivenDynamicpath and always falls back to"1"would still pass here.Suggested test case
{ name: "featuregate-enabled-configurationManagement-ForkAndReload-max-servers", unsupportedConfigOverrides: `{"maxDynamicServers":"7"}`, configurationManagement: operatorv1.IngressControllerConfigurationManagementForkAndReload, dcmEnabled: true, expectedEnv: notSet, }, + { + name: "featuregate-enabled-configurationManagement-Dynamic-max-servers", + unsupportedConfigOverrides: `{"maxDynamicServers":"7"}`, + dcmEnabled: true, + configurationManagement: operatorv1.IngressControllerConfigurationManagementDynamic, + expectedEnv: []envData{ + {"ROUTER_HAPROXY_CONFIG_MANAGER", true, "true"}, + {"ROUTER_MAX_DYNAMIC_SERVERS", true, "7"}, + {"ROUTER_BLUEPRINT_ROUTE_POOL_SIZE", true, "0"}, + }, + }, { name: "featuregate-enabled-configurationManagement-Dynamic-max-servers-lower-limit", unsupportedConfigOverrides: `{"maxDynamicServers":"0"}`, dcmEnabled: true, configurationManagement: operatorv1.IngressControllerConfigurationManagementDynamic,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/operator/controller/ingress/deployment_test.go` around lines 1165 - 1207, Add a happy-path test case to the table in deployment_test.go that verifies a positive maxDynamicServers override is honored for ConfigurationManagement = Dynamic: add an entry (e.g. name "featuregate-enabled-configurationManagement-Dynamic-max-servers-valid-value") with unsupportedConfigOverrides `{"maxDynamicServers":"5"}`, dcmEnabled: true, configurationManagement: operatorv1.IngressControllerConfigurationManagementDynamic, and expectedEnv set using envData to include {"ROUTER_HAPROXY_CONFIG_MANAGER", true, "true"}, {"ROUTER_MAX_DYNAMIC_SERVERS", true, "5"} (and keep {"ROUTER_BLUEPRINT_ROUTE_POOL_SIZE", true, "0"} if consistent with other Dynamic expectations) so the test will fail if the code ignores the provided maxDynamicServers override.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/operator/controller/ingress/deployment_test.go`:
- Around line 1165-1207: Add a happy-path test case to the table in
deployment_test.go that verifies a positive maxDynamicServers override is
honored for ConfigurationManagement = Dynamic: add an entry (e.g. name
"featuregate-enabled-configurationManagement-Dynamic-max-servers-valid-value")
with unsupportedConfigOverrides `{"maxDynamicServers":"5"}`, dcmEnabled: true,
configurationManagement:
operatorv1.IngressControllerConfigurationManagementDynamic, and expectedEnv set
using envData to include {"ROUTER_HAPROXY_CONFIG_MANAGER", true, "true"},
{"ROUTER_MAX_DYNAMIC_SERVERS", true, "5"} (and keep
{"ROUTER_BLUEPRINT_ROUTE_POOL_SIZE", true, "0"} if consistent with other Dynamic
expectations) so the test will fail if the code ignores the provided
maxDynamicServers override.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 9e1994e6-8485-4c4d-b746-2afac2e13b3a
📒 Files selected for processing (2)
pkg/operator/controller/ingress/deployment.gopkg/operator/controller/ingress/deployment_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/operator/controller/ingress/deployment.go
Contributor
|
@Miciah: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
Contributor
|
cc @ShudiLi |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Bump openshift/api for configurationManagement API
Bump to github.com/Miciah/api@6a8c07c9cf163d3b5e7964ffb5c7d70f6bdeff5e to get the
spec.tuningOptions.configurationManagementAPI field to allow enabling or disabling the Dynamic Configuration Manager.Implement configurationManagement API
Check the
spec.tuningOptions.configurationManagementAPI field, and only enable the Dynamic Configuration Manager if the field is set to "Dynamic".For now, the default behavior (when the field is omitted) is not to enable DCM. Once we are more confident that the feature is safe to enable, we will change the default behavior.
This PR depends on openshift/api#2757.